Rearrange document tabs via drag and drop#3707
Conversation
ac697ea to
2d1fa0a
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces drag-and-drop functionality for reordering document tabs, a great enhancement for user experience. The implementation spans both the Rust backend and the Svelte frontend, with a new ReorderDocument message and corresponding handler. The core drag-and-drop logic resides in the Panel.svelte component. While the feature is functional, I've identified a key area for improvement in the event listener management within Panel.svelte to boost performance and prevent potential conflicts. My review comment details this suggestion to help refine this proof-of-concept into a production-ready feature.
| onMount(() => { | ||
| addEventListener("pointermove", draggingPointerMove); | ||
| addEventListener("pointerup", draggingPointerUp); | ||
| addEventListener("mousedown", draggingMouseDown); | ||
| addEventListener("keydown", draggingKeyDown); | ||
| }); | ||
|
|
||
| onDestroy(() => { | ||
| removeEventListener("pointermove", draggingPointerMove); | ||
| removeEventListener("pointerup", draggingPointerUp); | ||
| removeEventListener("mousedown", draggingMouseDown); | ||
| removeEventListener("keydown", draggingKeyDown); | ||
| }); |
There was a problem hiding this comment.
Global event listeners for drag-and-drop are added in onMount and removed in onDestroy. This is inefficient as they are active throughout the component's lifecycle, even when no drag is occurring. This can lead to performance degradation and potential conflicts with other parts of the application.
A more performant and robust approach is to attach these listeners to the window object only when a drag operation begins (i.e., in tabPointerDown) and remove them once it's complete (in a cleanup function called from draggingPointerUp and other abort scenarios).
To implement this, you can:
- Remove the
onMountandonDestroylifecycle hooks for these listeners. - Add
window.addEventListener(...)forpointermove,pointerup,mousedown, andkeydowninsidetabPointerDown. - Add
window.removeEventListener(...)for the same events insideabortDrag.
|
@TrueDoctor I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/components/window/Panel.svelte">
<violation number="1" location="frontend/src/components/window/Panel.svelte:142">
P2: Accessing `dragState.startIndex` without a null check. `dragState` is typed `DragState | undefined` and the enclosing `if` condition doesn't narrow it. Add an explicit guard to avoid a potential runtime `TypeError`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| function draggingPointerUp() { | ||
| if (dragInPanel && dragDropIndex !== undefined) { | ||
| reorderAction?.(dragState.startIndex, dragDropIndex); |
There was a problem hiding this comment.
P2: Accessing dragState.startIndex without a null check. dragState is typed DragState | undefined and the enclosing if condition doesn't narrow it. Add an explicit guard to avoid a potential runtime TypeError.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/components/window/Panel.svelte, line 142:
<comment>Accessing `dragState.startIndex` without a null check. `dragState` is typed `DragState | undefined` and the enclosing `if` condition doesn't narrow it. Add an explicit guard to avoid a potential runtime `TypeError`.</comment>
<file context>
@@ -58,19 +74,139 @@
+
+ function draggingPointerUp() {
+ if (dragInPanel && dragDropIndex !== undefined) {
+ reorderAction?.(dragState.startIndex, dragDropIndex);
+ justFinishedDrag = true;
+ // Clear after the current tick so a same-tick click is still suppressed, but the next intentional click is not swallowed
</file context>
| reorderAction?.(dragState.startIndex, dragDropIndex); | |
| if (dragState) reorderAction?.(dragState.startIndex, dragDropIndex); |
5bb6104 to
52d2b38
Compare
Allows rearranging document tabs in the graphite ui using drag and drop.
Note this code has been ai generated. It has been tested to work, but is more intended as a proof of concept and potential blueprint for a proper pr.